Use single method to open/close flyout#542
Conversation
|
|
||
| _hideFlyout = (e) => { | ||
| this.opened = false; | ||
| this._close(); |
There was a problem hiding this comment.
Lit provides a native way to update dependent attributes/variables on reactive property changes. We should use that pattern instead of relying on manually calling methods like this. With willUpdate(), the current code would be fine
humitos
left a comment
humitos
left a comment
There was a problem hiding this comment.
Take a look at the tests I wrote and try to write a test case for this case. These cases where we need to interact with the elements to do QA are pretty hard to notice after deploy otherwise.
|
Cool... I'm a little lost in general with all of this, so might take a while to get back to this. |
|
@ericholscher let me know if I can help here or if you want me to take over it. |
|
Feel free to run with it. It was just a small bug I wanted to fix, but the PR feedback seems to require a ton of research that I don't have energy for currently. |
|
I think this is still a bug, and this PR makes things better, but I'm going to close it since folks didn't want to ship it. |
|
This is still a bug and Anthony opened a bye issue for it. It's not that we don't want to ship it, there is feedback about how to use the framework to solve this issue that wasn't addressed: #542 (comment) |
|
This logic that we have here now might also be the cause of #585. If we aren't always using the magic helpers |
|
Yea, it's definitely the same issue as #585. |
3 participants
Fixes #541